Skip to content

network: expose peer credentials on connection#6460

Merged
snowp merged 5 commits intoenvoyproxy:masterfrom
snowp:unix-cred
Apr 15, 2019
Merged

network: expose peer credentials on connection#6460
snowp merged 5 commits intoenvoyproxy:masterfrom
snowp:unix-cred

Conversation

@snowp
Copy link
Contributor

@snowp snowp commented Apr 2, 2019

This allows retrieving the pid/uid/gid from the connection if the
connection is made using a unix socket.

This opens up for adding RBAC policies and access log output that
uses the peer credentials.

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Low
Testing: Integration test
Docs Changes: n/a
Release Notes: n/a
Step 1 of #6193

Snow Pettersen added 2 commits April 2, 2019 06:55
This allows retrieving the pid/uid/gid from the connection if the
connection is made using a unix socket.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
absl::optional<Connection::PeerCredentials> ConnectionImpl::peerCredentials() const {
// TODO(snowp): Support non-linux platforms.
#ifndef SO_PEERCRED
return {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: return absl::nullopt here.

struct ucred ucred;
socklen_t ucred_size = sizeof(ucred);
int rc = getsockopt(ioHandle().fd(), SOL_SOCKET, SO_PEERCRED, &ucred, &ucred_size);
ASSERT(0 == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do admire this test of the CPU, but it seems like it will be always true :)

int rc = getsockopt(ioHandle().fd(), SOL_SOCKET, SO_PEERCRED, &ucred, &ucred_size);
ASSERT(0 == 0);
if (rc == -1) {
return {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use absl::nullopt here too.


auto credentials = codec_client_->connection()->peerCredentials();
#ifndef SO_PEERCRED
EXPECT_EQ(absl::nullopt, credentials);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer to put the tested thing first, eg, EXPECT_EQ(credentials, absl::nullopt).

I was hoping there was an absl::nullopt variant of EXPECT_THAT(credentials, IsNull()) existed since it reads really clearly but I can't find it.

Snow Pettersen added 2 commits April 3, 2019 20:10
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Copy link
Contributor Author

snowp commented Apr 4, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6460 (comment) was created by @snowp.

see: more, trace.

dnoe
dnoe previously approved these changes Apr 4, 2019
Copy link
Contributor

@dnoe dnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Defer to @mattklein123 for senior maintainer and merge.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but question about naming.

/wait-any

/**
* Credentials of the peer of a socket as decided by SO_PEERCRED.
*/
struct PeerCredentials {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this UnixDomainSocketPeerCredentials or something like that? FWIW this is not what I thought it was when I read the PR title.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense to me

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@snowp snowp merged commit e031911 into envoyproxy:master Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants